@Exempt-Medic You had a lot of issues with this. Could you peer review this?

@threeandthreee also you should probably also see this

I haven't been forced to look at fill stuff yet so I don't really understand how it works yet. Can't offer a particularly insightful review but it seems simple enough, thanks for looking into it.

I'll put it into the next beta release and spend some time gaining understanding in case my review becomes necessary.

I don't think this is correct in general. An all-state is a state that can (and has) reached every location in the multiworld (assuming there are no games with bugs that result in locations that are always unreachable).
If an LADX player were to have used item plando to place an item from the pool into a dungeon locked behind small keys, the all-state would have been able to reach that location and collect that item (because it had the small keys in its inventory to start with and they were only removed later). This would incorrectly allow the pre_fill fill_restrictive to place the small keys into locations that are locked behind the plando-ed item because the all-state has already collected that item.

I think the state that should be used should collect all items from the item pool and all pre-fill items belonging to players other than the LADX player that is currently in pre_fill. Generally, collecting all items from the item pool would be enough, but I think item plando can cause things to break if the state does not also include the pre-fill items belonging to other players.

Consider a dungeon where every location requires a Hammer because it's required to enter the dungeon in the first place. Item plando from pool is then used to place the Hammer in a different player's world, locked behind one of their pre-fill items. When trying to pre_fill into the dungeon that requires the Hammer, if the CollectionState does not have that other player's pre-fill items, and that other player's pre_fill has not been run yet, then the CollectionState cannot reach the Hammer, so none of the locations in the dungeon will be reachable and trying to pre_fill into it will raise a FillError.

I think most worlds don't collect the pre-fill items from other worlds when they do their pre_fill currently, though it's probably only item plando that can break things.

Removing the victory item from the all-state does work to make pre_fill perform proper placement though.

When sweeping for advancements, a CollectionState won't collect items from locations that it has already collected from previously, so even though the victory location will still be reachable to the all-state, it will refuse to pick up the victory item again.

I agree with the previous comments. To give a concrete example why the solution is not perfect yet, I added the following plando block:

  plando_items:
    - item:
        Tail Key: 1
      location: Spark, Mini-Moldorm Chest (Tail Cave)
      from_pool: true
      world: true
      percentage: 100

This is an incredibly bad plando, because you need the Tail Key to reach the location, so this always break.
However, we would want an error in fill_restrictive, and with the current solution, we get an "Unbeatable Game" error after placing all items.

Here is my proposed solution: on minimal accessibility: allow_partial, but check if game is beatable after filling of dungeons. If it is, do remaining_fill afterwards.

Here are several cases, some of which give intended errors.

• Default yaml, full accessibility: success, no errors.

• Default yaml, minimal accessibility: success, no errors (FIX!).

• Default yaml, full accessibility, with plando Tail Key in Tail Cave:

Fill.FillError: No more spots to place 7 items. Remaining locations are invalid.
Unplaced items:
Stone Beak (Tail Cave) (Player 1), Dungeon Map (Tail Cave) (Player 1), Compass (Tail Cave) (Player 1), Small Key (Tail Cave) (Player 1), Small Key (Tail Cave) (Player 1), Small Key (Tail Cave) (Player 1), Nightmare Key (Tail Cave) (Player 1)
All Placements:
[..., (Spark, Mini-Moldorm Chest (Tail Cave), Tail Key), ...]

This is the "correct" error; these items need to be in the dungeon, but cannot be made accessible, which violates the full accessibility setting.

• Default yaml, minimal accessibility, with plando Tail Key in Tail Cave

Fill.FillError: The game is unbeatable after filling dungeons.

This is a custom error. The items were all successfully placed. Some are not accessible, that that is fine because of minimal accessibility. However, with these placements, the game is not beatable. This error is only intended to occur when plando asks for impossible seeds.

• Default yaml, minimal accessibility, with plando Tail Key in Tail Cave, requiring 7 instruments (so Tail Cave is not required):
  Generation successful, no errors! (FIX!)
  Playthrough:

Unreachable Items:

Small Key (Tail Cave): Two Stalfos, Two Keese Chest (Tail Cave)
Full Moon Cello: Full Moon Cello (Tail Cave)
Nightmare Key (Tail Cave): Nightmare Key Chest (Tail Cave)
Small Key (Tail Cave): Mini-Moldorm Spawn Chest (Tail Cave)
Tail Key: Spark, Mini-Moldorm Chest (Tail Cave)
Seashell: Three of a Kind Chest (Tail Cave)
Small Key (Tail Cave): Hardhat Beetles Key (Tail Cave)

The unreachable items are intended because you can never enter Tail Cave because the Tail Key is plandod in there.

The point still stands that a full all-state cannot be used with fill_restrictive here.

My understanding is that the can_beat_game check is primarily there to check for impossible plandos and fail early, though this should be resolved with #3046 that uses fill_restrictive to place plando-ed items, which would prevent the impossible placement in the first place.

I think a correct all-state, that does not include the dungeon items being placed, can be constructed like this.

        # Set up state
        partial_all_state = CollectionState(self.multiworld)
        # Collect every item from the item pool and every pre-fill item like MultiWorld.get_all_state, but don't sweep.
        for item in self.itempool:
            partial_all_state.collect(item, prevent_sweep=True)
        for player in self.multiworld.player_ids:
            subworld = self.worlds[player]
            for item in subworld.get_pre_fill_items():
                partial_all_state.collect(item, prevent_sweep=True)
        # get_all_state would normally sweep here, but we need to remove our dungeon items first.

        # Remove dungeon items we are about to put in from the state so that we don't double count
        for item in all_dungeon_items_to_fill:
            partial_all_state.remove(item)

        # Sweep to pick up already placed items that are reachable with everything but the dungeon items.
        partial_all_state.sweep_for_advancements()

First let me paste your code that works for me, I had to change a few things (add .multiworld twice iirc) because I was getting errors.

        # Set up state
        partial_all_state = CollectionState(self.multiworld)
        # Collect every item from the item pool and every pre-fill item like MultiWorld.get_all_state, but don't sweep.
        for item in self.multiworld.itempool:
            partial_all_state.collect(item, prevent_sweep=True)
        for player in self.multiworld.player_ids:
            subworld = self.multiworld.worlds[player]
            for item in subworld.get_pre_fill_items():
                partial_all_state.collect(item, prevent_sweep=True)
        # get_all_state would normally sweep here, but we need to remove our dungeon items first.
        
        # Remove dungeon items we are about to put in from the state so that we don't double count
        for item in all_dungeon_items_to_fill:
            partial_all_state.remove(item)

        # Sweep to pick up already placed items that are reachable with everything but the dungeon items.
        partial_all_state.sweep_for_advancements()
        
        fill_restrictive(self.multiworld, partial_all_state, all_dungeon_locs_to_fill, all_dungeon_items_to_fill, lock=True, single_player_placement=True, allow_partial=False)

This code seems to work (too)! I will have to think about what the differences are, if any.

I'm not sure what the differences are, but your code also has all intended results and succeeded 1000 random yamls!
So I'll go ahead and use your solution, I don't understand it 100% but it makes sense.
I have updated the initial post too.

I had to change a few things (add .multiworld twice iirc) because I was getting errors.

Ah, whoops, I was copying code from MultiWorld.get_all_state, so it's just self in that code rather than self.multiworld that was needed in the LADX code.

What's being fixed

This partial all-state fixes cases where there are pre-placed (plando-ed or otherwise) items at locations locked behind items in all_dungeon_items_to_fill. See the image in #4281 (comment).

The problem with get_all_state

The state from multiworld.get_all_state() starts with all the items in self.get_pre_fill_items(), which should include all the items in all_dungeon_items_to_fill, and picks up any placed items that are locked behind those items.

Removing the items in all_dungeon_items_to_fill from all_state does not also remove all the items that can only be reached with the items being removed. That is simply not possible to do currently (if we could do it, it might make generation much faster).

The partial all-state

Instead, when creating the partial_all_state, all the items in all_dungeon_items_to_fill are removed from partial_all_state before the state has a chance to pick up any placed items. Then, when telling partial_all_state to pick up all reachable items with sweep_for_advancements, it won't be able to pick up any placed items locked behind items in all_dungeon_items_to_fill.

fill_restrictive

fill_restrictive works by removing one item from all_dungeon_items_to_fill, creating a copy of the state passed as an argument (partial_all_state) and then collecting all the remaining items from all_dungeon_items_to_fill into the copied state's inventory. The copied state is then told to pick up all reachable items that it has not already picked up (sweep_for_advancements) and then fill_restrictive finds a location in all_dungeon_locs_to_fill that it can place that one item that was removed from all_dungeon_items_to_fill. This process repeats until every item has been removed from all_dungeon_items_to_fill and (hopefully) placed or it runs out of locations in all_dungeon_locs_to_fill.

1. Remove an item from all_dungeon_items_to_fill, this is the item_to_place.
2. Copy partial_all_state, this is the maximum_exploration_state.
3. Collect all remaining items in all_dungeon_items_to_fill into maximum_exploration_state's inventory.
4. Make maximum_exploration_state pick up every placed item it can reach that it has not picked up before.
5. Look through all_dungeon_locs_to_fill and find a location that maximum_exploration_state can reach and item_to_place can be placed at
6. Place item_to_place at the found location.
7. Remove the location from all_dungeon_locs_to_fill.

These steps repeat until either all_dungeon_items_to_fill or all_dungeon_locs_to_fill are empty. Any items that cannot be placed at all, get put in an unplaced_items list. With allow_partial=True, the items in unplaced_items are added back into all_dungeon_items_to_fill before fill_restrictive returns. With allow_partial=False, if there are any items in unplaced_items, you get a FillError.

There's a bit more too it than that because it's slightly different when there is more than one player, and there's also 'swap' which can be used to try to fix placements when maximum_exploration_state can't reach any of the remaining locations or item_to_place can't be placed at any of the remaining locations.

I think it would be perfectly fine to still allow Minimal Accessibility to do a partial_fill=True followed by a remaining_fill if the game is beatable.

should probably just skip your own pre_fill_items() instead of collect+remove them imo

also since it's a single player placement do you even need to collect any pre_fill_items?

I haven't been able to look at this PR anymore since my last comment, I'm also perhaps not knowledgable enough anymore since the solution was expanded upon a lot.
In any way, I think @threeandthreee should approve this at least too before it should be merged. No hurry of course! They are also free to open their own PR for this and then I will close this one.

from #4281 (comment)

Consider a dungeon where every location requires a Hammer because it's required to enter the dungeon in the first place. Item plando from pool is then used to place the Hammer in a different player's world, locked behind one of their pre-fill items. When trying to pre_fill into the dungeon that requires the Hammer, if the CollectionState does not have that other player's pre-fill items, and that other player's pre_fill has not been run yet, then the CollectionState cannot reach the Hammer, so none of the locations in the dungeon will be reachable and trying to pre_fill into it will raise a FillError.

I think most worlds don't collect the pre-fill items from other worlds when they do their pre_fill currently, though it's probably only item plando that can break things.

@qwint does this not address the reason to collect pre fill items?

I really appreciate the detailed explanations Mysteryem. Its a lot so I don't pretend to fully understand but it was a huge help. Your reasoning makes sense to me and the current pr fixes the issue so this has my approval as is (for whatever value that currently has 😀).

There may be adjustments available but I don't really want to hold this up for them since its a functioning fix for a pretty big bug.

that does answer my side comment of "why collect other world's pre fill items" but not "why collect+remove your own instead of just skipping your world when iterating over pre_fill_items"

Unless I'm misunderstanding something it's only our dungeon items getting removed. Wouldn't we still want our pre fills for the same reason we want everyone else's?

are you pre filling outside of prefill? everything that should be in that list should either be filled by now or be the items you are currently filling

I'm not certain I understood you in the first place so let me back up. In the current PR are you saying that we should be skipping our own world while setting up partial_all_state?

If so, I don't understand why we would do that.

As Mysteryem explained, we're setting up partial_all_state just to get an all_state that doesn't include our dungeon items and things locked behind them.

get_pre_fill_items is meant to return the items a world places in pre_fill so that an all state constructed before pre_fill completes can contain all the items that will eventually be placed (because they are not in start inventory, itempool, or on filled locations, the places where all state can check)

if you are in your own pre_fill all relevant items should already be placed (since this is the last step of your pre_fill) or in itempool/start inventory, any items not fitting those categories should be items that you are going to place in the restrictive fill using that all state, so if you have them duplicated they can cause erroneous fills (the whole reason why you're removing at all)

and if they are somehow not in any of those groups they shouldn't be in your pre_fill_items and are likely not relevant to fill anyways (since they would never get placed in that case)

I think all the dungeon items to place in pre_fill here (all_dungeon_items_to_fill) are the same items that would be returned by self.get_pre_fill_items(). With these being the same, rather than removing the dungeon items from the partial_all_state afterwards, the same resulting partial_all_state would be achieved by skipping collecting the get_pre_fill_items() for the current world.

        # Set up state
        partial_all_state = CollectionState(self.multiworld)
        # Collect every item from the item pool and every pre-fill item like MultiWorld.get_all_state, except not our own pre-fill items.
        for item in self.multiworld.itempool:
            partial_all_state.collect(item, prevent_sweep=True)
        for player in self.multiworld.player_ids:
            if player == self.player:
                # Don't collect the items we're about to place.
                continue
            subworld = self.multiworld.worlds[player]
            for item in subworld.get_pre_fill_items():
                partial_all_state.collect(item, prevent_sweep=True)

        # Sweep to pick up already placed items that are reachable with everything but the dungeon items.
        partial_all_state.sweep_for_advancements()

If they are the same, then it does then question if creating all_dungeon_items_to_fill needs to be done at the start of pre_fill or if you can just do all_dungeon_items_to_fill = self.get_pre_fill_items() immediately before the line that sorts the items (all_dungeon_items_to_fill.sort(key=priority)).

That makes sense now, thank you